Skip to content

fix(a11y): composite alpha when measuring contrast ratios - #122

Open
KhyFee wants to merge 4 commits into
philaconvalley:mainfrom
KhyFee:fix/contrast-alpha-117
Open

fix(a11y): composite alpha when measuring contrast ratios#122
KhyFee wants to merge 4 commits into
philaconvalley:mainfrom
KhyFee:fix/contrast-alpha-117

Conversation

@KhyFee

@KhyFee KhyFee commented Aug 5, 2026

Copy link
Copy Markdown

Summary

Test plan

  • npx playwright test e2e/contrast.spec.ts

@vercel

vercel Bot commented Aug 5, 2026

Copy link
Copy Markdown

@KhyFee is attempting to deploy a commit to the PhilaCon Valley Team on Vercel.

A member of the Team first needs to authorize it.

@traksaw traksaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compositing fix itself is right: f.a * v + (1 - f.a) * b.rgb[i] is proper source-over, the ~2.62 expectation computes to 2.6179 (comfortably inside toBeCloseTo(2.62, 1)), and none of the three existing product assertions change — hero-cta, event-bar-cta, and the nav pill are all opaque text-brand-dark on bg-brand-pink/bg-accent-400, so the suite stays green on that front.

Requesting changes on one blocker: the branch fails npm run format:check, which .github/workflows/ci.yml:37 runs as a required step, so this lands red as-is.

Beyond that, the comments below are mostly about the helper reporting a passing ratio for a pairing that genuinely fails. I know the PR body defers semi-transparent backgrounds to a follow-up, and that's a fair scope call — my ask is only that the deferral be loud (throw) rather than silent (green check), so we don't bank confidence on measurements the helper can't actually make yet.

One more, outside the diff — effectiveColours, line 67

GitHub won't let me anchor an inline comment there since it's untouched by this PR, so it goes here.

isTransparent only rejects alpha === 0, so the walk-up stops at the first partially transparent ancestor and hands its raw RGB back as both the composite base and the background luminance. Concrete case: dark text over a bg-white/10 veil on the dark page measures 17.40:1 — passes AA comfortably — when the real composited ratio is 1.34:1.

This isn't hypothetical for the current DOM: src/components/Header.astro:168 uses bg-brand-dark/90.

Deferring full background compositing is fine, but as written the helper manufactures a passing number instead of declining to measure. Minimum viable version — keep climbing when the background isn't opaque, or throw:

const a = alpha && alpha.length === 4 ? Number(alpha[3]) : 1;
if (a > 0 && a < 1) {
  throw new Error(`translucent background ${bg} on <${node.tagName}> — not yet supported`);
}

Comment thread e2e/contrast.spec.ts Outdated
Comment thread e2e/contrast.spec.ts Outdated
Comment thread e2e/contrast.spec.ts
Comment thread e2e/contrast.spec.ts Outdated
Comment thread e2e/contrast.spec.ts Outdated
@KhyFee

KhyFee commented Aug 10, 2026

Copy link
Copy Markdown
Author

Addressed review notes on e2e/contrast.spec.ts:

  • Prettier (format:check) — sort + blank line
  • Normalize % alpha + reject out-of-range alpha so modern rgb(... / 85%) cannot silently invent huge ratios
  • Pin opaque contrast to 6.3965 instead of self-comparing through the same path
  • Drop dead parseRgb and the stray internal comment

Ready for another look when you have a moment — thank you for the careful review.

@traksaw traksaw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking #117 — the core compositing fix is right. I ran the new helper standalone and both new assertions hold (6.396487 vs toBeCloseTo(6.3965, 3), and 2.61796 vs ~2.62), and the opaque path is numerically identical to the old parseRgb, so there's no regression on the existing tests.

Two things to fix before merge — one real, one latent. Details inline.

Comment thread e2e/contrast.spec.ts Outdated
Comment thread e2e/contrast.spec.ts
function parseRgb(value: string): number[] {
const nums = value.match(/[\d.]+/g);
function parseRgba(value: string): { rgb: number[]; a: number } {
const nums = value.match(/[\d.]+%?/g);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new % support applies to the colour channels too, not just alpha, and misparses them silently.

parseRgba('rgb(100%, 40%, 66%)') returns rgb: [1, 0.4, 0.66], which relativeLuminance then divides by 255 — a bright pink is measured as near-black and the ratio is garbage, with no error raised. The same silent-nonsense path swallows any non-legacy serialization: color(srgb 1 0.4 0.66)[1, 0.4, 0.66], oklch(0.7 0.15 20)[0.7, 0.15, 20].

This is latent today (Tailwind 3.4 emits legacy rgb()/rgba() and Chromium serializes computed colours that way), but it's exactly the "modern syntax" the guard on line 37 claims to cover — and that guard never actually fires for 85%, since the map above already converted it to 0.85. Rejecting a % on any of the first three components, or a function name other than rgb/rgba, would make the failure loud instead of a wrong number.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — latest commit rejects % on RGB channels and non-rgb/rgba functions, while still converting percentage alpha.

@KhyFee

KhyFee commented Aug 17, 2026

Copy link
Copy Markdown
Author

Thanks @traksaw — appreciate the careful follow-up on the helper.

Addressed both open points in the latest commit:

  1. parseRgba now only accepts rgb()/rgba(), rejects % on RGB channels, and still converts percentage alpha (including modern rgb(... / 85%)).
  2. contrastRatio composites translucent backgrounds over page white, and throws on fully transparent backgrounds instead of reporting a false 21:1.

Added unit coverage for those cases. Ready for another look when you have a moment.

Comment thread e2e/contrast.spec.ts Outdated
if (b.a === 0) {
throw new Error(`transparent background "${bg}"`);
}
const bgRgb = b.a < 1 ? b.rgb.map((v) => b.a * v + (1 - b.a) * 255) : b.rgb;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contrastRatio always composites a translucent background over pure white (line 70), but effectiveColours's DOM walk-up (lines 81-94) only continues past a background when alpha is exactly 0. A background with alpha between 0 and 1, e.g. rgba(20,20,20,0.9), is accepted immediately and never resolved against its real ancestor backdrop. Example: a CTA with background: rgba(20,20,20,0.9) on a dark-navy section — effectiveColours returns that value right away, then contrastRatio blends it with white instead of navy, which can report a wrong AA pass or fail. The PR description flags this as deferred follow-up work, so this is a known scope cut, not an oversight — flagging in case you want it closed before merge.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — latest commit composites translucent ancestor layers instead of treating 0 < a < 1 as opaque and then blending that over white.

@KhyFee

KhyFee commented Aug 19, 2026

Copy link
Copy Markdown
Author

Thanks @traksaw — good catch, and you’re right it was a known cut.

Closed it in the latest commit: effectiveColours now walks past 0 < alpha < 1 layers and composites each over the next opaque ancestor (then page white), so a translucent CTA on navy is measured against navy, not white. contrastRatio still composites a bare translucent bg over white for the unit-test path.

Happy to tweak the flatten if you want it to stop only at a === 1 with a different fallback.

@KhyFee

KhyFee commented Aug 19, 2026

Copy link
Copy Markdown
Author

Composite alpha fix is in per last review — standing by if anything else should move to a follow-up PR vs this one.

@KhyFee

KhyFee commented Aug 26, 2026

Copy link
Copy Markdown
Author

Thanks again @traksaw — appreciate you staying on the compositing edge cases.

Latest on the branch (also rebased onto current main):

  1. parseRgba rejects % on RGB channels and non-rgb()/rgba() functions
  2. Translucent backgrounds composite over white (or throw when fully transparent)
  3. effectiveColours walks past 0 < alpha < 1 layers and composites over ancestors before page white

Ready for another look when you have a moment. Thanks!

@KhyFee
KhyFee force-pushed the fix/contrast-alpha-117 branch from 24a3ba7 to e60590b Compare August 26, 2026 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Contrast test helper drops the alpha channel, so translucent text is scored as opaque

2 participants